fix: Add a fallback matching mechanism for output_files#1237
fix: Add a fallback matching mechanism for output_files#1237alopezz wants to merge 2 commits intobazel-contrib:mainfrom
Conversation
alopezz
changed the title
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d024a28af
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| for file in files_list: | ||
| if file.path == "/".join([ctx.bin_dir.path, ctx.attr.target.label.workspace_root, path]): | ||
| return file |
There was a problem hiding this comment.
The new fallback reconstructs a path under ctx.bin_dir.path, then compares it to file.path. That only works for generated outputs in the bin tree. For external repos that expose source files via DefaultInfo (e.g., filegroup(srcs = glob(...)) in an http_archive), file.path is under external/<repo>/... with no bazel-out/.../bin prefix, so this fallback still never matches and output_files continues to fail for the common “external repo sources” case the change is targeting. You likely need an additional check against the workspace-root execution path (or other roots) so source files can be found too.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
That's indeed true (I've manually confirmed). Even though it looks like a less likely use case, I see no reason not to cover it as well, I fixed it on a second commit.
1 participant
This PR proposes a fallback mechanism for the
output_filesrule, to make it more usable with external repositories.Specifically, I was trying to select files from the
DefaultInforeturned by a rule implemented on a build file applied to an http_archive source and failed until I discovered that theshort_pathfor these files is a relative path containing the canonical repo name, for example:The documentation for
output_filesreferences select_file from skylib, but the implementation of that rule seems to just match based on the ending (endswith), which is much more relaxed but also more convenient to use.This attempts a middle ground, where it keeps the existing mechanism based on
short_path(ensuring backwards compatibility), while adding a fallback mechanism that reconstructs the execution path such that it can match to thepath; this works at least for the specific case that I was working with.